@lazychaser Can this be merged?

@lazychaser can you take a look?

@lazychaser can you merge this?

@lazychaser can you take a look please?

If you could have a look it would be nice. The changelog isn’t that big as I see it.

@julesgraus you might want to rebase the branch because the L12 compatibility was merged.

Maybe then @jonnott would have a second to look at it.

Done

@julesgraus Do you think putting this out as v6.1.0 would be a good plan?

@lazychaser https://github.com/lazychaser/laravel-nestedset/releases/tag/untagged-9ddb651ef9d4ba790dc6

@jonnott I just looked at the PR you merged, and because there are also signature changes involved (some methods are now typed that weren't before), technically, 7.0.0 release would actually be correct.

For example this change:
newNestedSetQuery($table = null) → newNestedSetQuery(?string $table = null)

Means, that if in case somebody passed a table name as int instead of string, they'd now get a TypeError (a little far fetched, but still valid). And that's why it's required to be marked as Major release, which expresses that there are breaking changes in the API (method signatures).

Another way is to specifically revert the changes where no type was declared before, and keep the other fixes fixing the deprecations. This would allow 6.1.0 release.

@jonnott I just looked at the PR you merged, and because there are also signature changes involved (some methods are now typed that weren't before), technically, 7.0.0 release would actually be correct.

For example this change: newNestedSetQuery($table = null) → newNestedSetQuery(?string $table = null)

Means, that if in case somebody passed a table name as int instead of string, they'd now get a TypeError (a little far fetched, but still valid). And that's why it's required to be marked as Major release, which expresses that there are breaking changes in the API (method signatures).

Another way is to specifically revert the changes where no type was declared before, and keep the other fixes fixing the deprecations. This would allow 6.1.0 release.

@julesgraus Thoughts on what @kohlerdominik says?

I am not using or near a computer at the moment. Technically what @kohlerdominik says is true and valid. But the docblocks above the methods already said that you need to pass in strings or null.

So yeah, the signature has changed and it now throws type errors, but you have been warned before by the docblock if you used it in a wrong way.

And most importantly: it probable did not work anyway if you used the methods wrong. Regardless if this technically is a breaking change, the user had his app in a broken state already if he did use the methods wrong.

So @kohlerdominik is right. But even though, without looking into it any further, I do not think it’s a problem for the users of the package.

what about versioning? If we could agree for now that it isn’t a major change, it would be a patch release I guess (the last number in the version needs to increment). Because it’s not a feature (middle number of the version.

If you guys decide that it must be a major release anyways, let me know. I can apply the changes on a version 7 branch for example. Just let me know.

I think it could be a minor release then - i.e. 6.1.0. It's not a breaking change technically as you say, as if you weren't adhering to the correct types on these methods, your app would be broken anyway.

Yes, I agree 👍

Ok. I have the privileges to tag a new version, but I'm worried about breaking peoples' apps by getting anything wrong which introduces a BC break. Thoughts appreciated..

@jonnott when merging #619 you'd get these changes between 6.0.5 and the next release: v6.0.5...ysfks:laravel-nestedset:fix/php84-warnings

Allowing the next release to be 6.0.6.